Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #10108: Add auto save conflict dialog #10426

Merged
merged 12 commits into from
Aug 28, 2019

Conversation

maxme
Copy link
Contributor

@maxme maxme commented Aug 21, 2019

Fixes #10108: Add and auto save conflict dialog, Demo video

Related FluxC PR: wordpress-mobile/WordPress-FluxC-Android#1357

Here is the dialog, with current copy:

Screenshot_1566489119

For reference, In Calypso:

screenshot-2019-05-03-at-09 25 19

Note the "Saved draft" is not 100% correct, since the post was published, that's why I didn't keep it.

@osullivanchris what do you think about that? Should we ping editorial directly?

To test:

  • Create a post in wpandroid and publish it.
  • Edit this post in Calypso (Add "Calypso, Calypso, Calypso" to the content), close the browser tab without saving.
  • Refresh the Post list in wpandroid.
  • You should see a "Unhandled auto save" label.
  • Tap edit, conflict should show up, pick "current version".
  • Don't edit the post, tap back.
  • "Unhandled auto save" label should remain.
  • Tap edit, conflict should show up, pick "more recent version".
  • Make sure the post looks like what you modified in Calypso ("Calypso, Calypso, Calypso").
  • Modify the post (add "WPAndroid, WPAndroid, WPAndroid"), tap back (this should auto save the post)
  • "Unhandled auto save" should be gone, you should see a "Local changes" label.
  • Open this post on Calypso, tap "Restore", make sure you see "WPAndroid, WPAndroid, WPAndroid" in the post content.

@maxme maxme requested a review from malinajirka August 21, 2019 16:54
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Aug 21, 2019

Warnings
⚠️ PR is not assigned to a milestone.

Generated by 🚫 dangerJS

@maxme maxme marked this pull request as ready for review August 22, 2019 16:22
@peril-wordpress-mobile
Copy link

You can test the changes on this Pull Request by downloading the APK here.

Copy link
Contributor

@malinajirka malinajirka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @maxme! Really great work ;)!

The code looks great. I have left a few comments but they are all related to one thing - usage of word "conflict".

However, I've encountered one "bug"

  1. Create an unpublished revision in Calypso
  2. Open Post List in the app
  3. Notice the "Unhandled auto save" label
  4. Click on the post
  5. Choose "More recent version"
  6. Click back (don't change the post)
  7. Click on the post again
  8. Choose "Current version"
  9. Notice the content of the post is "More recent version"

@osullivanchris
Copy link

@maxme I added the copy review label. I think what you have written does a pretty good job of explaining it. But perhaps they can wave a little magic to make it more concise/simple.

If you need to go ahead and ship it we can make the copy change an improvement afterwards

@benhuberman
Copy link

benhuberman commented Aug 24, 2019

Hi @osullivanchris @maxme. It would be nice to make this clearer / user-friendlier if we could -- to me it sounds confusing to have to choose between "Current" and "Most recent," since the two can mean the exact same thing.

Just to make sure I understand the context here: if a user publishes a post on WPAndroid, edits (but doesn't save the edits) in Calypso, and then returns to WPAndroid, they'll get this message when trying to open the post for more editing. Is that... more or less correct?

If so, below are my suggested edits -- let me know if these work for you; I tried to, as much as possible, to center this around what we can reasonably expect the user to know about their own post. If you think these aren't quite there either, thanks for helping me better understand the context in which this message will appear and I'd be happy to tweak the copy accordingly. :)

Instead of...

More recent version conflict

How about...

Which version would you like to edit?

And, instead of...

A more recent version exists. Edit current version or most recent version?

How about...

You recently made changes to this post but didn't save them. Would you like to...

And in the CTAs, instead of

Current version / More recent version

How about:

Continue with unsaved changes / Discard unsaved changes

@malinajirka
Copy link
Contributor

I'll be taking over this ticket as Maxime left for vacation and we'd like to merge the PR.

Thanks @benhuberman!

It would be nice to make this clearer / user-friendlier if we could -- to me it sounds confusing to have to choose between "Current" and "Most recent," since the two can mean the exact same thing.

Yep, I agree it's a bit confusing. The "current" is referring to the "current" version on the phone, whereas the "Most recent" to a most recent version from Calypso.

  1. User edits a published post in WPAndroid, but doesn't save it
  2. User edits the same post in Calypso, but doesn't save it
  3. User opens the post in WPAndroid - they have two unsaved version "current" version on WPAndroid or "Mot recent" version (the one from Calypso)
    But I agree using "current/most recent" doesn't help.

How about:
Continue with unsaved changes / Discard unsaved changes

This is the only part I'm not sure about. As per the steps above there can be two unsaved versions and it might scare the user that they'll lose both unsaved revisions. What about something like "Continue with local/Load most recent web version"?

@osullivanchris
Copy link

Thanks @benhuberman for taking a look!

I agree with @malinajirka it all sounds good to me, except the actions.

Both versions are 'equal' they were just made on different platforms at different times. I think we had a version of this dialogue that shows the times things were edited @malinajirka ?

I think it would be more useful to get them to choose a version by tapping on it directly and then we wouldn't need a button label to try and explain it in so many words. Something like
"Choose a version"
"Local (date and time)"
"Web (date and time)"
with blue text or some other affordance to let the user know the item itself is interactive

@malinajirka
Copy link
Contributor

Good idea! I'll re-use the look of the conflict resolution dialog.

Screenshot 2019-08-26 at 12 25 05

@benhuberman
Copy link

I really like the direction that lets a user explicitly choose one of two specific versions. My remaining concern is about how we name these versions. Both "Local" and "Web" don't sound immediately clear to me. I can mull this a bit more and will let you know if/when I think of more intuitive CTAs.

@benhuberman
Copy link

Following up here, I wonder if the two options below might work -- or are at least steps in the right direction. The copy teeing them up would still be:

You recently made changes to this post but didn't save them. Would you like to...

Then, the two new choices would be:

Continue with the unsaved version on this app

And...

Continue with the unsaved version from another device

I know that "a different device" is tricky -- but "web" still strikes me as more confusing / open to mis-interpretation.

@malinajirka
Copy link
Contributor

I really like using "another device" instead of "web". As it can theoretically be a version from a tablet or an iPhone.
My only remaining concern is that the texts are quite long, considering they'll be used as buttons.

How about

"Which version would you like to edit?"
"You recently made changes to this post but didn't save them. Would you like to continue with"
"The version on this app"
"The version from another device"

I'm not sure if it's the best idea to have a button without a verb... but it'd solve some UI issues on smaller devices.
I'd at least consider omitting the word "unsaved" as imho it doesn't add any value.

Wdyt?

@osullivanchris
Copy link

Took a quick look at how these options could look and made some tweaks to make it work in the UI. Option 2 looks best to me - being able to both see the details about the version and click directly on it to select it.

The concepts are already abstract so I like being able to click directly on an item rather than describing it, and then having a separate text button that you have to click (and figure out which one it relates to).

Maybe we could say 'on this app' and 'from another device'. On and from might help explain that one is here and the other is elsewhere? not sure

Screenshot 2019-08-27 at 10 43 04

@malinajirka
Copy link
Contributor

Thanks Chris!

Option 2 looks best to me - being able to both see the details about the version and click directly on it to select it.

I hate to say this :(, but both options 2 and 3 would require a custom dialog implementation.
They contain formatted text (not support in the default dialog). Option 2 also contains text below the buttons which also isn't supported in the default dialog.
I believe that's the main reason why the existing conflict resolution dialog looks as it looks https://user-images.githubusercontent.com/2261188/63684299-d4f4a680-c7fc-11e9-983a-170f8293278a.png
We can theoretically implement a custom dialog, but we need to consider whether it's worth it and if we can even afford to implement it in the first iteration.

@osullivanchris
Copy link

@malinajirka thanks for letting me know! I don't think this is worth a custom implementation right now either.

Option 1 and 3 should still be valid, if you remove the text stylings from option 3.

Option 2 with tap-able rows would be out though.

@malinajirka malinajirka merged commit 9865c55 into master-auto-upload Aug 28, 2019
@malinajirka
Copy link
Contributor

I'm merging this PR into a feature branch since I don't want to mix Maxime's and mine changes. I'll create a new PR for the remaining issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants